Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CI workflow #10

Merged
merged 13 commits into from
Oct 24, 2024
Merged

Update CI workflow #10

merged 13 commits into from
Oct 24, 2024

Conversation

cjdsellers
Copy link
Collaborator

@cjdsellers cjdsellers commented Oct 19, 2024

Upgrades the action versions and black in the ci.yml workflow, and adds Python 3.13 to the build matrix.

As a test, I've also taken the liberty of bumping up the MSRV from 1.63 to 1.70 so that the latest tokio can be supported, which fixes the MSRV CI job. (I could do this in a separate PR, or we could adjust the approach here.)

I've also made the bold move of dropping Python 3.7 and 3.8 in CI. (I can also revert this.)

@cjdsellers
Copy link
Collaborator Author

I'm unsure what's going on with this error for macos runners (some versions):

Error: dyld[3002]: Library not loaded: /usr/local/opt/gettext/lib/libintl.8.dylib

I found a similar issue here.

This definitely needs a squash-merge at this point too (if we merge).

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sorry I missed this for a couple days. Overall looks good to me, below are a few quick thoughts / suggestions:

@@ -160,7 +153,7 @@ jobs:
needs: [fmt]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
with:
toolchain: nightly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably update the coverage job to use cargo-llvm-cov and stable Rust (like PyO3 does). Possibly a follow-up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, I think it would be best to align with PyO3's practices. I'd like to take another pass at CI once we get the first release of pyo3-async-runtimes out the door.

- python-version: pypy-3.9
platform: { os: "windows-latest", python-architecture: "x86" }
include:
# Test minimal supported Rust version
- rust: 1.63.0
- rust: 1.70.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question on the MSRV bump. In PyO3 we support ranges of optional dependencies and have a helper script to downgrade them for the MSRV job to compatible versions. I'm unsure what the best option is here. Keeping the MSRV down is useful for linux distributions putting these packages in their package managers, e.g. PyO3/pyo3#3113

We last did an MSRV bump with PyO3 0.22 to 1.63, which is the current version supported by Debian stable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basing the MSRV on Debian stable and aligning it with PyO3's requirements, rather than targeting the latest version required by tokio, feel like the right approach (I'll revert back to 1.63). I'm just not sure of the best way to set this up in CI to avoid the error with tokio. The helper script sounds like what we need, but I'll need some time to figure it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now seems to be the last issue with CI, other than some warnings I'd like to address on another pass (some of the Rust toolchain installation actions are very outdated).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I can perhaps try to port that script tomorrow 👍

Comment on lines 62 to 63
python-architecture: "x64",
rust-target: "x86_64-apple-darwin",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

macos-latest runners are now arm:

Suggested change
python-architecture: "x64",
rust-target: "x86_64-apple-darwin",
python-architecture: "arm64",
rust-target: "aarch64-apple-darwin",

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed up the merge conflict, I assume this is good now. Thanks!

@davidhewitt davidhewitt merged commit d5bdc3f into PyO3:main Oct 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants